feat: implemented mobile header, fixed item sizing on mobile#163
feat: implemented mobile header, fixed item sizing on mobile#163negimox wants to merge 2 commits into
Conversation
|
@negimox is attempting to deploy a commit to the djed-solidity Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds standardized responsive breakpoints and mixins, global box-sizing and html/body resets, and numerous SCSS responsive adjustments across app and component styles to refine header, layout, navigation, and component sizing/spacing for tablet and mobile viewports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/molecules/BuySellButton/BuySellButton.scss (1)
66-69: Remove empty placeholder classes.Lines 66-69 contain empty CSS classes that serve no styling purpose and should either be implemented with styles or removed entirely. These appear to be placeholders that have been left in production code.
🧹 Nitpick comments (5)
src/components/atoms/CustomButton/_CustomButton.scss (1)
20-42: Prefer responsive mixins over hardcoded media queries.The responsive styling is logically sound, but the file imports
_general_styling.scsswhich provides reusable responsive mixins (@mixin on-tablet,@mixin on-mobile) that match these exact breakpoints. Using the mixins would improve maintainability and reduce duplication across the codebase.Consider refactoring to use:
@include on-tablet { .CustomButton { height: 45px; // ... } } @include on-mobile { .CustomButton { height: 40px; // ... } }This pattern is consistent with the responsive design system introduced in this PR.
src/components/molecules/BuySellCoin/_BuySellCoin.scss (1)
51-92: Font sizes may be too small on smallest viewport; verify readability.The responsive scaling down to 10px (AdditionalInfo at line 88) on the smallest breakpoint (425px) may impact legibility. While the PR aims to fit content on mobile, ensure that at least 12px (or your minimum accessibility standard) is maintained for body text.
Additionally, as with other component files, consider using the responsive mixins (
@mixin on-tablet,@mixin on-mobile) rather than hardcoded media queries for consistency with the new responsive design system.src/components/molecules/BuySellButton/BuySellButton.scss (1)
33-64: Responsive scaling is well-designed; align with responsive mixins system.The progressive responsive adjustments across three breakpoints (768px, 600px, 425px) are logically sound and follow a consistent pattern. However:
The 600px breakpoint is a custom addition not part of the standardized breakpoint set in
_general_styling.scss($breakpoint-md is 600px but the mixins may not clearly map to this). Consider whether this should use a named mixin for clarity.Continue using
@mixin on-tablet,@mixin on-tablet-sm, and@mixin on-mobileinstead of hardcoded media queries for consistency.src/App.scss (1)
334-417: Mobile header implementation is well-structured; verify z-index and positioning.The mobile header implementation introduces a sophisticated z-index hierarchy (1000, 1001, 1003) for stacking the dropdown, header, and icons. The structure appears correct:
.menu-mobile-dropdown: z-index 1000 (overlay).ant-layout-header.header-desktop: hidden at 600px.header-mobile: z-index 1001 (header layer)- Logo/icons: z-index 1003 (above dropdown)
However, verify the following:
Positioning brittleness: The dropdown's fixed
topposition (72px at 600px breakpoint, 62px at 425px) assumes a specific header height. If header padding changes, the dropdown position becomes misaligned. Consider using JavaScript to calculate the offset dynamically or using a more flexible approach (e.g., CSS custom properties).Responsive mixins: Continue using
@mixin on-tablet-smand@mixin on-mobilefor consistency instead of hardcoded media queries.src/scss/_general_styling.scss (1)
16-52: Excellent responsive design system foundation.The breakpoint variables and responsive mixins are well-structured and provide a solid foundation for responsive design. The five breakpoints cover the typical responsive spectrum (mobile, tablet, desktop, large desktop), and the naming convention is clear and intuitive.
However, note: While this system is defined here, the component files across the PR (CustomButton, BuySellCoin, MetamaskStatusButton, BuySellButton, and App.scss) all use hardcoded
@mediaqueries instead of the provided mixins. This creates a "defined but underutilized" pattern.Recommendation: In follow-up PRs, refactor component files to use these mixins consistently (e.g.,
@include on-mobile { ... }instead of@media (max-width: 425px) { ... }). This will improve maintainability and ensure breakpoints are centrally managed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/App.scss(3 hunks)src/components/atoms/CustomButton/_CustomButton.scss(1 hunks)src/components/molecules/BuySellButton/BuySellButton.scss(1 hunks)src/components/molecules/BuySellCoin/_BuySellCoin.scss(1 hunks)src/components/molecules/MetamaskStatusButton/_MetamaskStatusButton.scss(2 hunks)src/index.css(1 hunks)src/scss/_general_styling.scss(1 hunks)
🔇 Additional comments (4)
src/index.css (1)
1-18: Verifyoverflow-x: hiddennecessity; may mask mobile scroll issues.Setting
overflow-x: hiddenglobally is a strong constraint that will hide horizontal overflow. While this prevents unintended horizontal scrolling, it can also mask mobile layout issues where content legitimately overflows. Ensure this is intentional and that mobile components don't have content that needs horizontal scrolling (e.g., tables, carousels).Additionally,
height: 100%onhtmlandbodymay create rigid layout constraints if content grows beyond the viewport. Consider whethermin-height: 100vhwould be more appropriate for layouts with flexible content.src/components/atoms/CustomButton/_CustomButton.scss (1)
12-12: Good addition ofwhite-space: nowrap.This prevents text wrapping in buttons and improves layout stability on mobile.
src/App.scss (2)
16-16: Excellent responsive logo sizing withflex-shrink: 0andheight: auto.The logo scaling across breakpoints (80px → 40px → 35px) with proper flex properties prevents unwanted compression and maintains aspect ratio. This is a well-implemented responsive component.
Also applies to: 20-20, 29-30
376-410: Verify dropdown menu overlay width and mobile behavior.The dropdown menu is set to
width: 100vw(line 381) withposition: fixedpositioned at left/right 0. While this creates a full-width overlay, verify:
- Interaction with the global
overflow-x: hidden(may prevent scrollable content within dropdown if needed)- Whether the dropdown should allow vertical scrolling if menu items exceed viewport height (currently no scroll directive on
.ant-dropdown-menu)- The
background: rgba(0, 0, 0, 0.95)overlay may hide content behind it unexpectedly on long menusConsider adding
max-heightandoverflow-y: autoto.menu-mobile-dropdownif the menu items might exceed viewport height.
|
Hey, @Zahnentferner @yogesh0509 could you please review the PR? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
For issue #162


Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.